[Explore] Add auto scroll when new line is added #10977
Conversation
Signed-off-by: abbyhu2000 <[email protected]>
abbyhu2000
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
Maosaic,
SuZhou-Joe,
ZilongX,
ananzh,
angle943,
ashwin-pc,
bandinib-amzn,
curq,
d-buckner,
huyaboo,
joshuali925,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ps48,
raintygao,
ruanyl,
sejli,
virajsanghvi,
wanglam,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
abbyhu2000
requested review from
TackAdam,
paulstn,
ruchidh and
sumukhswamy
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10977 +/- ##
=======================================
Coverage 60.79% 60.79%
=======================================
Files 4531 4531
Lines 122258 122265 +7
Branches 20498 20503 +5
=======================================
+ Hits 74322 74334 +12
+ Misses 42693 42689 -4
+ Partials 5243 5242 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: abbyhu2000 <[email protected]>
abbyhu2000
dismissed stale reviews from sejli, ruchidh, and angle943
via
7238b6f
WalkthroughChanges add automatic scrolling functionality to the query panel editor. When new content is added and exceeds the visible area, the editor automatically scrolls to reveal the last line. Updates include a changelog entry, test mocks supporting line operations, and implementation of the auto-scroll behavior in the editor's content change handler. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changelogs/fragments/10977.yml(1 hunks)src/plugins/explore/public/components/query_panel/query_panel_editor/use_query_panel_editor/use_query_panel_editor.test.ts(1 hunks)src/plugins/explore/public/components/query_panel/query_panel_editor/use_query_panel_editor/use_query_panel_editor.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (1)
changelogs/fragments/10977.yml (1)
1-2: LGTM!The changelog entry is properly formatted and accurately documents the auto-scroll feature.
| getModel: jest.fn(() => ({ getLineCount: jest.fn() })), | ||
| revealLine: jest.fn(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test coverage for the auto-scroll feature.
While the mocks for getModel and revealLine are correctly added, there's no test verifying the new auto-scroll behavior. The existing test at lines 432-457 validates layout and scrollbar visibility but doesn't assert that revealLine is called when content exceeds maxHeight.
Add a test case to verify the auto-scroll behavior:
it('should auto-scroll to the last line when content exceeds max height', () => {
const mockGetLineCount = jest.fn(() => 10);
mockEditor.getModel.mockReturnValue({ getLineCount: mockGetLineCount });
mockEditor.getContentHeight.mockReturnValue(150); // Exceeds maxHeight of 100
const { result } = renderHook(() => useQueryPanelEditor());
act(() => {
result.current.editorDidMount(mockEditor);
});
// Trigger content size change
const contentSizeChangeCallback = mockEditor.onDidContentSizeChange.mock.calls[0][0];
act(() => {
contentSizeChangeCallback();
});
expect(mockGetLineCount).toHaveBeenCalled();
expect(mockEditor.revealLine).toHaveBeenCalledWith(10);
});🤖 Prompt for AI Agents
In
src/plugins/explore/public/components/query_panel/query_panel_editor/use_query_panel_editor/use_query_panel_editor.test.ts
around lines 147-148, add a new test that mounts the hook and asserts the
auto-scroll behavior: mock editor.getModel to return an object whose
getLineCount returns 10, set mockEditor.getContentHeight to return a value
greater than maxHeight (e.g. 150), call renderHook to create the hook, invoke
result.current.editorDidMount(mockEditor), retrieve and call the registered
onDidContentSizeChange callback from
mockEditor.onDidContentSizeChange.mock.calls[0][0] inside act, then assert the
getLineCount mock was called and mockEditor.revealLine was called with 10 to
verify auto-scroll to the last line.
|
|
||
| // Automatically scroll to the bottom when new lines are added | ||
| if (contentHeight > finalHeight) { | ||
| const lastLine = editor.getModel()?.getLineCount() || 0; | ||
| editor.revealLine(lastLine); | ||
| } |
There was a problem hiding this comment.
Auto-scroll may disrupt users reviewing earlier content.
The current implementation auto-scrolls on any content size change when content exceeds maxHeight, without checking the user's current scroll position. If a user scrolls up to review earlier content, any content size change (even unrelated to typing) will force them back to the bottom, disrupting their workflow.
Best practice: Only auto-scroll when the user is already near the bottom of the editor. You can check the current scroll position before calling revealLine.
Consider this approach:
// Automatically scroll to the bottom when new lines are added
if (contentHeight > finalHeight) {
+ const scrollTop = editor.getScrollTop();
+ const scrollHeight = editor.getScrollHeight();
+ const layoutInfo = editor.getLayoutInfo();
+ const isNearBottom = scrollTop + layoutInfo.height >= scrollHeight - 50; // 50px threshold
+
+ if (isNearBottom) {
const lastLine = editor.getModel()?.getLineCount() || 0;
editor.revealLine(lastLine);
+ }
}This ensures auto-scroll only occurs when the user is already viewing content near the bottom, preserving their position if they've scrolled up intentionally.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Automatically scroll to the bottom when new lines are added | |
| if (contentHeight > finalHeight) { | |
| const lastLine = editor.getModel()?.getLineCount() || 0; | |
| editor.revealLine(lastLine); | |
| } | |
| // Automatically scroll to the bottom when new lines are added | |
| if (contentHeight > finalHeight) { | |
| const scrollTop = editor.getScrollTop(); | |
| const scrollHeight = editor.getScrollHeight(); | |
| const layoutInfo = editor.getLayoutInfo(); | |
| const isNearBottom = scrollTop + layoutInfo.height >= scrollHeight - 50; // 50px threshold | |
| if (isNearBottom) { | |
| const lastLine = editor.getModel()?.getLineCount() || 0; | |
| editor.revealLine(lastLine); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/plugins/explore/public/components/query_panel/query_panel_editor/use_query_panel_editor/use_query_panel_editor.ts
around lines 316 to 321, the current auto-scroll unconditionally reveals the
last line when contentHeight exceeds finalHeight, which forces users back to the
bottom even if they scrolled up; modify the logic to first inspect the editor's
current scroll position (or visible range) and only call
editor.revealLine(lastLine) when the user is already near the bottom (for
example within a small threshold of the bottom in pixels or lines), otherwise do
nothing so their manual scroll position is preserved.
4 participants
Description
Add auto scroll when new line is added so we see the line we are typing.
Issues Resolved
Screenshot
before:
Screen.Recording.2025-11-25.at.4.12.38.PM.mov
after:
Screen.Recording.2025-11-25.at.4.14.55.PM.mov
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.